Documentation: add documentation for non-experimental environment variables - #9227
Documentation: add documentation for non-experimental environment variables#9227ulascansenturk wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9227 +/- ##
=======================================
Coverage 87.54% 87.54%
=======================================
Files 429 429
Lines 30622 30622
=======================================
+ Hits 26807 26808 +1
+ Misses 3814 3813 -1
Partials 1 1 🚀 New features to boost your workflow:
|
| Variables whose names contain `EXPERIMENTAL` are intentionally not documented | ||
| here: they guard features that are still in development, and they may change | ||
| behavior, change defaults, or be removed entirely in any release without | ||
| notice. |
There was a problem hiding this comment.
Are you intending to say that the documented list here is exhaustive other than EXPERIMENTAL flags?
There was a problem hiding this comment.
Yes, that's the intent, and your question caught that it wasn't actually true. I swept every os.Getenv/os.LookupEnv call across the main module and all submodules and found four non-experimental variables missing: GOOGLE_CLOUD_PROJECT (read by gcp/observability, alongside the two config variables already listed) and CSM_CANONICAL_SERVICE_NAME, CSM_WORKLOAD_NAME, CSM_MESH_ID (read by stats/opentelemetry/csm). All four are now documented.
I also made the exhaustiveness claim explicit rather than implied, and broadened the exclusion note to cover TEST_ONLY names as well as EXPERIMENTAL ones. That accounts for GRPC_TEST_ONLY_GOOGLE_C2P_RESOLVER_TRAFFIC_DIRECTOR_URI, which exists for gRPC's own tests. The 15 variables already in the list all check out against the code, with no stale entries.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a new documentation file, environment_variables.md, which lists and describes the environment variables supported by the grpc-go implementation. The review feedback suggests updating the logging section to include the FATAL severity level in the documentation for GRPC_GO_LOG_SEVERITY_LEVEL to ensure completeness.
| logger. One of `ERROR`, `WARNING` or `INFO` (e.g. `INFO` enables info, | ||
| warning and error logs). Defaults to `ERROR`. Has no effect if the |
There was a problem hiding this comment.
The FATAL log level is also supported by the default logger's environment variable parser (it maps to fatalLog in grpclog). We should include FATAL in the list of supported severity levels to make the documentation complete.
| logger. One of `ERROR`, `WARNING` or `INFO` (e.g. `INFO` enables info, | |
| warning and error logs). Defaults to `ERROR`. Has no effect if the | |
| logger. One of `FATAL`, `ERROR`, `WARNING` or `INFO` (e.g. `INFO` enables info, | |
| warning, error and fatal logs). Defaults to `ERROR`. Has no effect if the |
There was a problem hiding this comment.
Not taking this one, as FATAL isn't a recognized value. The parser at grpclog/loggerv2.go:66 has cases for only "", ERROR, WARNING and INFO (plus lowercase forms) and no default, so FATAL matches nothing and leaves infoW, warningW and errorW at io.Discard. Since grpclog/internal/loggerv2.go:258 derives the fatal writer as fatalW := errorW, setting FATAL produces no output at any severity rather than fatal-only logging.
It does point at a real gap in the doc, though: an unrecognized value silences the default logger entirely instead of falling back to the ERROR default, which is worth calling out. I've added a note to that effect, including that Fatal* calls still exit the process in that case since exit(1) runs unconditionally after the (discarded) print.
State that the list is exhaustive and broaden the exclusion note to cover TEST_ONLY variables alongside EXPERIMENTAL ones, then add the non-experimental variables that were missing: GOOGLE_CLOUD_PROJECT and the three CSM_* labels read by stats/opentelemetry/csm. Also document that an unrecognized GRPC_GO_LOG_SEVERITY_LEVEL silences the default logger entirely rather than falling back to the default.
7848987 to
995de8b
Compare
| * `GRPC_GO_LOG_SEVERITY_LEVEL` | ||
|
|
||
| The minimum severity of log messages written to stderr by the default | ||
| logger. One of `ERROR`, `WARNING` or `INFO` (e.g. `INFO` enables info, |
There was a problem hiding this comment.
We should mention the behaviour in case of an unrecognized value, there are no default fallbacks.
There was a problem hiding this comment.
Added. There is no fallback: an unrecognized value silences the logger entirely rather than leaving ERROR in effect, so no message of any severity is written. Called that out explicitly, along with FATAL being one such value.
| logger. One of `ERROR`, `WARNING` or `INFO` (e.g. `INFO` enables info, | ||
| warning and error logs). Defaults to `ERROR`. Has no effect if the | ||
| application replaces the default logger via | ||
| [`grpclog.SetLoggerV2`](https://pkg.go.dev/google.golang.org/grpc/grpclog#SetLoggerV2). |
There was a problem hiding this comment.
This applies to other flags as well, so may be we can move it to common section above for the entire Logging block.
There was a problem hiding this comment.
Moved. The Logging preamble now says the whole section configures the default logger and has no effect once the application installs its own via SetLoggerV2, and that the variables are read once at package init. Dropped the per variable mention.
| This document lists the environment variables supported by the grpc-go | ||
| implementation. | ||
|
|
||
| This list is intended to be exhaustive, with two deliberate exclusions: |
There was a problem hiding this comment.
The repo officially supports two more environment variables that this list
misses: HTTPS_PROXY and NO_PROXY (case-insensitive), documented in
Documentation/proxy.md and read via
http.ProxyFromEnvironment in
internal/resolver/delegatingresolver/delegatingresolver.go:42 (the lookup is
made with an https-scheme request, so HTTP_PROXY is not consulted, and
localhost targets are always excluded). Since this list claims to be
exhaustive, suggest adding a short Proxy section linking to
Documentation/proxy.md — the same pattern the Logging section uses with
log_levels.md. Alternatively, a third exclusion bullet could cover variables
whose semantics are defined outside gRPC, which would also account for
GOOGLE_APPLICATION_CREDENTIALS and the OTEL_* resource variables read
indirectly by the observability plugins.
There was a problem hiding this comment.
Did both, since they cover different cases.
Added a Proxy section for HTTPS_PROXY and NO_PROXY linking to Documentation/proxy.md, noting that the name matching is case-insensitive, that the lookup uses an https-scheme request so HTTP_PROXY is never consulted, and that a localhost host bypasses the proxy whether or not it is listed in NO_PROXY.
Also added a third exclusion bullet for variables whose semantics are defined outside gRPC and are only read through a dependency, which covers GOOGLE_APPLICATION_CREDENTIALS and the OTEL_* ones. The proxy variables did not seem to belong there since the behaviour around them is gRPC's own and already documented in the repo.
| * `GRPC_GO_LOG_FORMATTER` | ||
|
|
||
| Set to `json` to make the default logger emit log messages as JSON | ||
| objects. Any other value uses the default plain-text format. |
There was a problem hiding this comment.
Nit: matching is strings.EqualFold(..., "json") (grpclog/loggerv2.go:81),
so JSON/Json etc. also work — could say "Set to json (case-insensitive)"
to mirror the precision elsewhere in the doc.
There was a problem hiding this comment.
Fixed, now reads "Set to json (case-insensitive)".
| Whether the DNS resolver ignores errors from TXT record lookups. When | ||
| `true`, TXT lookup failures are logged but resolution proceeds without a | ||
| service config; when `false`, the error is reported to the channel. | ||
| Defaults to `true`. |
There was a problem hiding this comment.
This description is inverted relative to the code
(internal/resolver/dns/dns_resolver.go:293-303 and handleDNSError at
:278-291). When true (the default), the TXT lookup error is dropped and
lookupTXT returns nil immediately — nothing is logged; the logger.Info call
is inside handleDNSError, which for TXT errors is only reached in the false
branch. And when false, only transient errors reach the channel: permanent
DNS errors (e.g. a missing TXT record) are suppressed by handleDNSError
returning nil, so resolution still proceeds without a service config.
| Whether the DNS resolver ignores errors from TXT record lookups. When | |
| `true`, TXT lookup failures are logged but resolution proceeds without a | |
| service config; when `false`, the error is reported to the channel. | |
| Defaults to `true`. | |
| Whether the DNS resolver ignores errors from TXT record lookups. When | |
| `true` (the default), TXT lookup errors are silently ignored and | |
| resolution proceeds without a service config. When `false`, transient | |
| errors (timeouts and temporary failures) are logged and reported to the | |
| channel, and resolution is retried with backoff; permanent DNS errors, | |
| such as a missing TXT record, are still silently treated as "no service | |
| config". Defaults to `true`. |
There was a problem hiding this comment.
You are right, it was backwards. lookupTXT returns nil immediately when TXTErrIgnore is set, so nothing is logged in the true case, and handleDNSError (which holds the logger.Info call) is only reached in the false branch, where it also swallows permanent errors and returns nil.
Taken your wording almost verbatim.
| The maximum number of concurrent ALTS handshakes. Defaults to `100`; | ||
| values are clamped to the range `[1, 100]`. |
There was a problem hiding this comment.
nit: the limit is enforced per direction — separate client and server
semaphores are each sized to this value
(credentials/alts/internal/handshaker/handshaker.go:62-63), so a process that
performs both client- and server-side handshakes can have up to twice this
number in flight. Might be worth a sentence.
There was a problem hiding this comment.
Added. Confirmed there are two separate semaphores, clientHandshakes and serverHandshakes, each sized to the variable, so a process doing both can have up to twice this many in flight.
| The GCP project ID to report observability data against. If unset, the | ||
| project ID is taken from the default credentials. |
There was a problem hiding this comment.
Small precision gap: this variable is only consulted when the observability
config does not itself specify project_id —
ensureProjectIDInObservabilityConfig (gcp/observability/config.go:143-146)
is the sole caller of fetchDefaultProjectID. So the precedence is: config
project_id → GOOGLE_CLOUD_PROJECT → default credentials. Suggest: "Only
consulted when the observability config does not specify project_id. If this
variable is also unset, the project ID is taken from the default credentials."
There was a problem hiding this comment.
Added. ensureProjectIDInObservabilityConfig only calls fetchDefaultProjectID when config.ProjectID is empty, so the precedence is config project_id, then this variable, then the default credentials. Worded it that way.
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
Adds
environment_variables.mdat the repository top level, documenting the non-experimental environment variables supported by grpc-go, grouped by area (logging, binary logging, name resolution, xDS, load balancing, security, server, GCP observability).Variables containing
EXPERIMENTALin their names are intentionally left out, with a note explaining they may change or be removed without notice.Fixes #9213
RELEASE NOTES: none